Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support opaque mode for log_newpages #8582

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Conversation

knizhnik
Copy link
Contributor

@knizhnik knizhnik commented Aug 1, 2024

Problem

DuckDB pages do not contains page header.
See https://neondb.slack.com/archives/C0799BZALLR/p1722148143356959?thread_ts=1722030414.537519&cid=C0799BZALLR

The problem: we need to wallog huge (256kb) DuckDB pages.
It is natural to use Postgres log_newpages() function which insert FPI record.
The problem is that this function assumes that pages has Postgres PageHeader.
It is already has std_page parameter (where pd_lower,pg_upper specify hole inside page).
But even non-standard page is assumed to have PageHeader and Postgres is setting LSN for it.

So there are two other alternatives:

  1. Split DuckDB 256kb page in 33 Postgres 8kb pages excluding header. Last page will be almost empty. And overall this approach looks ugly.
  2. Add special resource manager for DuckDB. It requires much more coding and cause writing a lot of boilerplate and duplication of a lot of code from xlog.c

First looks ugly, second requires no changes in Postgres, but a lot of copy&paste of Postgres and Neon code.

Summary of changes

Add REGBUF_OPAQUE and pass flags to wal_newpages instead of std_page parameter.

Correspondent PR for Postgres:
neondatabase/postgres#456

Checklist before requesting a review

  • I have performed a self-review of my code.
  • If it is a core feature, I have added thorough tests.
  • Do we need to implement analytics? if so did you add the relevant metrics to the dashboard?
  • If this PR requires public announcement, mark it with /release-notes label and add several sentences in this section.

Checklist before merging

  • Do not forget to reformat commit message to not include the above checklist

@knizhnik knizhnik requested review from a team as code owners August 1, 2024 20:14
@knizhnik knizhnik requested review from arpad-m and ololobus August 1, 2024 20:14
Copy link

github-actions bot commented Aug 1, 2024

2108 tests run: 2039 passed, 0 failed, 69 skipped (full report)


Code coverage* (full report)

  • functions: 32.9% (7156 of 21726 functions)
  • lines: 50.6% (57779 of 114232 lines)

* collected from Rust tests only


The comment gets automatically updated with the latest test results
bb86bd2 at 2024-08-05T12:50:57.291Z :recycle:

@ololobus ololobus requested a review from hlinnaka August 2, 2024 10:56
@@ -1,5 +1,5 @@
{
"v16": ["16.3", "b39f316137fdd29e2da15d2af2fdd1cfd18163be"],
"v16": ["16.3", "2e372c392105e709cca8d9224c9c0a1fa5bd3aff"],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please

Cannot review the code part of the PR, so added Heikki as well

@knizhnik knizhnik force-pushed the log_opaque_newpages branch from c0b8fc9 to c82f094 Compare August 5, 2024 11:51
@knizhnik knizhnik force-pushed the log_opaque_newpages branch from c82f094 to 0f85575 Compare August 5, 2024 11:55
@ololobus ololobus marked this pull request as draft January 10, 2025 13:13
@ololobus
Copy link
Member

This PR has been automatically marked as Draft because it was last updated 158 days ago. Please, consider closing it if it's not needed anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants